Add example code for each configuration to verify correctness during continuous integration #12

mangs · 2024-01-03T23:37:10Z

Pull Request Checklist

I have read the CONTRIBUTING document
Readme and changelog updates were made reflecting this PR's changes
Increase the project version number in package.json and package-lock.json following Semantic Versioning

Changes Included

Version bump to 1.2.0
Testing added using example code for each configuration to verify correctness during continuous integration
React and Preact TypeScript configs now allow JSX in files with .tsx file extensions
Remove dev dependency npm-run-all because it was only used during the reinstall NPM script and would cause an error if dependencies weren't installed prior to execution

…continuous integration

…ot config

mangs

Some notes for context:

mangs · 2024-01-05T22:32:52Z

CHANGELOG.md

+## 1.2.0
+
+- Testing added using example code for each configuration to verify correctness during continuous integration
+- React and Preact TypeScript configs now allow JSX in files with `.tsx` file extensions


This improvement resulted from writing the tests

mangs · 2024-01-05T22:35:34Z

config/prettier/prettierConfig.json

@@ -20,7 +20,7 @@

  "overrides": [
    {
-      "files": "*.{cjs,js,jsx,mjs,ts,tsx}",
+      "files": "*.{cjs,cts,js,jsx,mjs,mts,ts,tsx}",


I was originally using .mts files when I started this work. I started off using Bun to write unit tests then settled on using example code and running ESLint against it because it was more informative and easier to identify the root cause of errors.

mangs · 2024-01-05T22:36:07Z

lib/eslintReactTypescriptConfig.json

@@ -9,5 +9,7 @@
    "browser": false,
    "node": false
  },
-  "rules": {}
+  "rules": {
+    "react/jsx-filename-extension": ["error", { "extensions": [".jsx", ".tsx"] }]


This is the improvement to the library that resulted from testing. The Preact Typescript config extends this config so it benefits from this too.

mangs · 2024-01-05T22:37:59Z

package.json

+    "@playwright/test": "1.40.1",
+    "@types/aws-lambda": "8.10.131",
+    "@types/node": "20.10.6",
+    "@types/react": "18.2.46",
+    "@types/react-dom": "18.2.18",
+    "eslint": "8.56.0",
+    "jest": "29.7.0",
+    "preact": "10.19.3",
+    "prettier": "3.1.0",
+    "prop-types": "15.8.1",
+    "react": "18.2.0",
+    "react-dom": "18.2.0",
+    "typescript": "5.3.3"


I only added dev dependencies necessary for these tests. I'll update non-dev dependencies in a separate PR to bring them up to their latest versions.

mangs · 2024-01-05T22:39:40Z

test/eslintBaseConfig/exampleBaseConfigEntrypoint.mjs

This was a huge time investment because every config extends from this one. I went one-by-one through every rule and tried to write code to tackle each, but it wasn't always possible. The most targeted rules ended up being the regex and unicorn ones.

mangs · 2024-01-05T22:41:03Z

test/eslintIsomorphicConfig/.browserslistrc

+[ssr]
+maintained Node versions


During this process, I learned you can target SSR separately using Browserslist. Food for thought.

mangs · 2024-01-05T22:43:08Z

test/eslintJestConfig/exampleJestConfigEntrypoint.js

This might look familiar 🙃

mangs · 2024-01-05T22:45:47Z

test/eslintPreactConfig/examplePreactConfigEntrypoint.jsx

Because I was running low on time, I adapted the same example for the Preact, Preact TypeScript, React, and React TypeScript tests

jduthon

Really nice work!

I think the last test examples are more similar to what I had imagined, basically they just allow to test that the config works overall (e.g. the eslint config is valid, it doesn't have broken parts and we can run it an ESLint check with it successfully).
The other one is interesting as it could give more safety, but I think it's quite hard to make it work correctly and maintain, so it might not be worth it I think.

Approving regardless of what we decide on the base tests, as ultimately I think they still do the job even though they might be a bit too complex or not necessarily 100% fulfilling their intended goals.

jduthon · 2024-01-08T13:59:40Z

test/eslintBaseConfig/exampleBaseConfigEntrypoint.mjs

+  objectWithSortedKeys,
+  oneThousand,
+  printEslintConfig,
+};


Quite thorough work here, but I am not sure whether this actually tests the rule.
As an example, if I were to remove the rule unicorn/prefer-node-protocol, tests for this file would still pass, because it wouldn't break any new rules.
I guess one way we could check for those could be by disabling things, and then as long as we have "report unused disables", we would get warned in case something goes wrong.

Yea this is a drawback to this approach. I had originally started with the ESLint API directly to be able to catch some negative cases (e.g. if a rule is disabled), but the way it reports back problems isn't very friendly so when I extrapolated the amount of work necessary to make it maintainable and easily understandable, I decided to just use the CLI which is far more friendly and what people are used to. Not perfect but a lot better than what was there before.

jduthon · 2024-01-08T14:01:52Z

test/eslintBrowserConfig/exampleBrowserConfigEntrypoint.mjs

+}
+
+// Begin Execution
+main();


In this file I can see you haven't specified which rules different parts of the code tests, is that intentional?

I mostly did that for the base config, then when I was time boxing I wasn't as thorough. Apparently I didn't add any here.

mangs added 22 commits January 3, 2024 18:35

Add example code for each configuration to verify correctness during …

3d46198

…continuous integration

added browser config test

06e1a3a

added isomorphic config test

d503ef6

scaffold out remaining tests

a0c4ba1

remove Bun test files

84e63b0

remove jsconfig.json because we have a tsconfig now

7fff375

add root: true to root ESLint config

fe0218a

remove npm-run-all

7d92e3f

add Jest dev dependency to fix Jest config test

76c9828

add real Node test code

611789f

added real Playwright test code

c4b27e1

add tests to CI

3b7370e

added real code for Typescript config test

48043d8

ts test tweaks

0316adb

added full test for React config

82c1dca

minor tweaks

29cfdda

completed React Typescript tests

af62d94

add preact to dev deps

e8b4b2d

add Preact config tests

6c1f34e

fix mistake in Preact config test

affd9a6

complete Preact Typescript tests

cac611f

add missing file extensions for tests

2f62a31

mangs marked this pull request as ready for review January 5, 2024 22:31

mangs requested a review from jduthon as a code owner January 5, 2024 22:31

make it clear the Preact TypeScript tsconfig is extending from the ro…

2f33e9b

…ot config

mangs commented Jan 5, 2024

View reviewed changes

jduthon approved these changes Jan 8, 2024

View reviewed changes

mangs merged commit 58d8dff into babbel:main Jan 9, 2024
1 check passed

mangs deleted the PNAPL-366-add-correctness-checks-for-each-config branch January 9, 2024 15:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add example code for each configuration to verify correctness during continuous integration #12

Add example code for each configuration to verify correctness during continuous integration #12

mangs commented Jan 3, 2024 •

edited

Loading

mangs left a comment

mangs Jan 5, 2024

mangs Jan 5, 2024

mangs Jan 5, 2024

mangs Jan 5, 2024

mangs Jan 5, 2024

mangs Jan 5, 2024

mangs Jan 5, 2024

mangs Jan 5, 2024

jduthon left a comment

jduthon Jan 8, 2024

mangs Jan 9, 2024

jduthon Jan 8, 2024

mangs Jan 9, 2024

		[ssr]
		maintained Node versions

Add example code for each configuration to verify correctness during continuous integration #12

Add example code for each configuration to verify correctness during continuous integration #12

Conversation

mangs commented Jan 3, 2024 • edited Loading

mangs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jduthon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mangs commented Jan 3, 2024 •

edited

Loading